Skip to content
This repository was archived by the owner on May 22, 2025. It is now read-only.

Comments

Fix snowflake stage not being deleted on error in load_file#1321

Merged
sunank200 merged 9 commits intomainfrom
delete-snowflake-stage
Nov 30, 2022
Merged

Fix snowflake stage not being deleted on error in load_file#1321
sunank200 merged 9 commits intomainfrom
delete-snowflake-stage

Conversation

@feluelle
Copy link
Member

@feluelle feluelle commented Nov 29, 2022

Description

What is the current behavior?

Currently the snowflake stage is not being dropped incase of an exception when calling load_file.

closes: #1262

What is the new behavior?

Drop snowflake stage also when file load failed.

Does this introduce a breaking change?

No.

Checklist

  • Created tests which fail without the change (if possible)
  • Extended the README / documentation, if necessary

@codecov
Copy link

codecov bot commented Nov 29, 2022 •
edited
Loading

Codecov Report

Base: 95.71% // Head: 94.99% // Decreases project coverage by -0.72% ⚠️

Coverage data is based on head (abfebd9) compared to base (b725430).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1321      +/-   ##
==========================================
- Coverage   95.71%   94.99%   -0.73%     
==========================================
  Files          19       71      +52     
  Lines         677     3315    +2638     
  Branches       68      382     +314     
==========================================
+ Hits          648     3149    +2501     
- Misses         18      102      +84     
- Partials       11       64      +53
Impacted Files Coverage Δ
python-sdk/src/astro/databases/snowflake.py 95.52% <100.00%> (ø)
python-sdk/src/astro/files/types/__init__.py 86.20% <0.00%> (ø)
python-sdk/src/astro/files/base.py 100.00% <0.00%> (ø)
python-sdk/src/astro/utils/load.py 92.30% <0.00%> (ø)
python-sdk/src/astro/files/locations/http.py 100.00% <0.00%> (ø)
python-sdk/src/astro/sql/operators/cleanup.py 97.97% <0.00%> (ø)
...sdk/src/astro/sql/operators/upstream_task_mixin.py 85.71% <0.00%> (ø)
python-sdk/src/astro/files/locations/__init__.py 100.00% <0.00%> (ø)
python-sdk/src/astro/sql/operators/load_file.py 96.62% <0.00%> (ø)
python-sdk/src/astro/sql/operators/transform.py 87.09% <0.00%> (ø)
... and 43 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

sunank200
sunank200 approved these changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@sunank200 sunank200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

kaxil
kaxil reviewed Nov 29, 2022
View reviewed changes
Copy link
Collaborator

@kaxil kaxil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the test is failing - https://github.com/astronomer/astro-sdk/actions/runs/3572950837/jobs/6007534322

feluelle
feluelle commented Nov 29, 2022
View reviewed changes
python-sdk/src/astro/databases/snowflake.py
except AttributeError:
try:
rows = self.hook.run(sql_statement)
except (AttributeError, ValueError) as exe:
Copy link
Member Author

@feluelle feluelle Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the AttributeError from here. Could anyone explain to me why we are catching these?

Copy link
Member Author

@feluelle feluelle Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaxil @utkarsharma2 @sunank200 maybe?

Copy link
Contributor

@sunank200 sunank200 Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it now its working without AttributeError

sunank200
sunank200 suggested changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@sunank200 sunank200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AttributeError would be required as this is raised by snowflake for which we should fallback if enabled by the user. Related to #1324

@sunank200 sunank200 force-pushed the delete-snowflake-stage branch from fdbff1f to 53ceb15 Compare November 30, 2022 10:14
@sunank200
Copy link
Contributor

sunank200 commented Nov 30, 2022

AttributeError would be required as this is raised by snowflake for which we should fallback if enabled by the user. Related to #1324

Tested it without AttributeError and it works so the change is fine

@sunank200 sunank200 merged commit 7e829af into main Nov 30, 2022
@sunank200 sunank200 deleted the delete-snowflake-stage branch November 30, 2022 10:52
sunank200 added a commit that referenced this pull request Dec 1, 2022
@feluelle @rajaths010494 @sunank200
Fix snowflake stage not being deleted on error in load_file (#1321)
e40dcbc 
# Description

## What is the current behavior?

Currently the snowflake stage is not being dropped incase of an
exception when calling `load_file`.

closes: #1262

## What is the new behavior?

Drop snowflake stage also when file load failed.

## Does this introduce a breaking change?

No.

### Checklist
- [x] Created tests which fail without the change (if possible)
- [ ] Extended the README / documentation, if necessary

Co-authored-by: rajaths010494 <rajath.srinivasaiah@astronomer.io>
Co-authored-by: Ankit Chaurasia <8670962+sunank200@users.noreply.github.com>
(cherry picked from commit 7e829af)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dimberman dimberman Awaiting requested review from dimberman

@utkarsharma2 utkarsharma2 Awaiting requested review from utkarsharma2

@pankajastro pankajastro Awaiting requested review from pankajastro pankajastro is a code owner

@pankajkoti pankajkoti Awaiting requested review from pankajkoti

3 more reviewers

@kaxil kaxil kaxil left review comments

@tatiana tatiana tatiana approved these changes

@sunank200 sunank200 sunank200 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Snowflake stage not deleted when load_file() fails

5 participants

@feluelle @sunank200 @tatiana @kaxil @rajaths010494